fix: adjust payload to customized managed sponsor forms#958
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe sponsor customized form reducer now initializes ChangesSponsor Customized Form Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reducers/sponsors/sponsor-customized-form-reducer.js`:
- Around line 46-52: The current construction of entity spreads payload.response
and maps payload.response.items and it.images directly, which will throw if
items or images are null/undefined; update the logic in the entity creation (the
block that builds entity from payload.response and uses
payload.response.items.map and it.images.map) to use defensive checks/optional
chaining and defaults (e.g., treat payload.response.items as an empty array and
each it.images as an empty array before mapping) so mapping never runs on
null/undefined and preserves existing behavior for present arrays.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e2f79e4-8787-4990-841c-cafb425e32d2
📒 Files selected for processing (1)
src/reducers/sponsors/sponsor-customized-form-reducer.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| return { | ||
| ...state, | ||
| entity: payload.response | ||
| const entity = { |
There was a problem hiding this comment.
@tomrndom
The fix addresses the bug for one data path (entity-level items from getSponsorManagedForm) but leaves the same bug active on the individual-item edit path (currentItem from getSponsorCustomizedFormItem), meaning images will still be silently dropped in the most common editing scenario. The missing regression test also means the fix is unverified.
| entity: payload.response | ||
| const entity = { | ||
| ...payload.response, | ||
| items: (payload.response.items || []).map((it) => ({ |
There was a problem hiding this comment.
@tomrndom
Add a unit test for RECEIVE_SPONSOR_CUSTOMIZED_FORM in sponsor-customized-form-reducer.js covering: items with images (file_path mapped), items with no images, and empty items array
| @@ -43,10 +43,17 @@ const sponsorCustomizedFormReducer = (state = DEFAULT_STATE, action) => { | |||
| return DEFAULT_STATE; | |||
There was a problem hiding this comment.
@tomrndom
DEFAULT_ENTITY missing items field
fix: Add items: [] to DEFAULT_ENTITY
smarcet
left a comment
There was a problem hiding this comment.
@tomrndom
Incomplete fix ( same image-drop bug on the individual-item edit path)
The primary edit flow for items goes through getSponsorCustomizedFormItem → RECEIVE_SPONSOR_CUSTOMIZED_FORM_ITEM → currentItem; that reducer case stores images raw (no file_url → file_path mapping), so saveSponsorFormManagedItem → normalizeManagedItem (line 1446: filter((img) => img.file_path)) still silently drops all existing images on save
src/reducers/sponsors/sponsor-customized-form-items-list-reducer.js, RECEIVE_SPONSOR_CUSTOMIZED_FORM_ITEM case (line 108) — currentItem stores raw API images without the mapping
Suggested fix
Apply the same images: (item.images || []).map(img => ({ ...img, file_path: img.file_url })) transform to currentItem in that case
… form item reducer Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reducers/sponsors/__tests__/sponsor-customized-form-reducer.test.js`:
- Around line 58-93: Add a new unit test that dispatches
RECEIVE_SPONSOR_CUSTOMIZED_FORM to sponsorCustomizedFormReducer with a payload
whose response.items contains an item object that omits the images property
(e.g., { id: 10, name: "Item A" }), and assert that
result.entity.items[0].images is an empty array; use the same DEFAULT_STATE
setup and naming pattern as the existing tests to ensure the reducer's fallback
for missing images is validated.
In `@src/reducers/sponsors/sponsor-customized-form-items-list-reducer.js`:
- Around line 113-117: The reducer reads item.meta_fields.length without
guarding for a missing meta_fields which can throw; change the meta_fields
normalization to mirror images by first defaulting meta_fields to an empty array
when falsy and then using the length check — e.g., treat (item.meta_fields ||
[]) as the source for the length test and return either the original
item.meta_fields when non-empty or an empty array otherwise so the reducer never
reads .length of undefined (refer to the item.meta_fields expression in the
sponsor-customized-form-items-list-reducer).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b9d3745-3eac-4fc2-8dbf-c618dbc923ab
📒 Files selected for processing (3)
src/reducers/sponsors/__tests__/sponsor-customized-form-reducer.test.jssrc/reducers/sponsors/sponsor-customized-form-items-list-reducer.jssrc/reducers/sponsors/sponsor-customized-form-reducer.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/reducers/sponsors/sponsor-customized-form-reducer.js
| it("handles items with no images", () => { | ||
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | ||
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | ||
| payload: { | ||
| response: { | ||
| id: 1, | ||
| code: "FORM1", | ||
| items: [{ id: 10, name: "Item A", images: [] }] | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| expect(result.entity.items[0].images).toEqual([]); | ||
| }); | ||
|
|
||
| it("handles empty items array", () => { | ||
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | ||
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | ||
| payload: { | ||
| response: { id: 1, code: "FORM1", items: [] } | ||
| } | ||
| }); | ||
|
|
||
| expect(result.entity.items).toEqual([]); | ||
| }); | ||
|
|
||
| it("handles missing items field", () => { | ||
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | ||
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | ||
| payload: { | ||
| response: { id: 1, code: "FORM1" } | ||
| } | ||
| }); | ||
|
|
||
| expect(result.entity.items).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Add a test for items where images is missing (not just empty).
You already validate images: [], but the reducer also handles images being absent. A direct test for that case will lock the intended fallback contract.
Suggested test case
it("handles items with no images", () => {
@@
expect(result.entity.items[0].images).toEqual([]);
});
+ it("handles items with missing images field", () => {
+ const result = sponsorCustomizedFormReducer(DEFAULT_STATE, {
+ type: RECEIVE_SPONSOR_CUSTOMIZED_FORM,
+ payload: {
+ response: {
+ id: 1,
+ code: "FORM1",
+ items: [{ id: 10, name: "Item A" }]
+ }
+ }
+ });
+
+ expect(result.entity.items[0].images).toEqual([]);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("handles items with no images", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { | |
| id: 1, | |
| code: "FORM1", | |
| items: [{ id: 10, name: "Item A", images: [] }] | |
| } | |
| } | |
| }); | |
| expect(result.entity.items[0].images).toEqual([]); | |
| }); | |
| it("handles empty items array", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { id: 1, code: "FORM1", items: [] } | |
| } | |
| }); | |
| expect(result.entity.items).toEqual([]); | |
| }); | |
| it("handles missing items field", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { id: 1, code: "FORM1" } | |
| } | |
| }); | |
| expect(result.entity.items).toEqual([]); | |
| }); | |
| it("handles items with no images", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { | |
| id: 1, | |
| code: "FORM1", | |
| items: [{ id: 10, name: "Item A", images: [] }] | |
| } | |
| } | |
| }); | |
| expect(result.entity.items[0].images).toEqual([]); | |
| }); | |
| it("handles items with missing images field", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { | |
| id: 1, | |
| code: "FORM1", | |
| items: [{ id: 10, name: "Item A" }] | |
| } | |
| } | |
| }); | |
| expect(result.entity.items[0].images).toEqual([]); | |
| }); | |
| it("handles empty items array", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { id: 1, code: "FORM1", items: [] } | |
| } | |
| }); | |
| expect(result.entity.items).toEqual([]); | |
| }); | |
| it("handles missing items field", () => { | |
| const result = sponsorCustomizedFormReducer(DEFAULT_STATE, { | |
| type: RECEIVE_SPONSOR_CUSTOMIZED_FORM, | |
| payload: { | |
| response: { id: 1, code: "FORM1" } | |
| } | |
| }); | |
| expect(result.entity.items).toEqual([]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors/__tests__/sponsor-customized-form-reducer.test.js`
around lines 58 - 93, Add a new unit test that dispatches
RECEIVE_SPONSOR_CUSTOMIZED_FORM to sponsorCustomizedFormReducer with a payload
whose response.items contains an item object that omits the images property
(e.g., { id: 10, name: "Item A" }), and assert that
result.entity.items[0].images is an empty array; use the same DEFAULT_STATE
setup and naming pattern as the existing tests to ensure the reducer's fallback
for missing images is validated.
|
|
||
| const currentItem = { | ||
| ...item, | ||
| images: (item.images || []).map((img) => ({ |
There was a problem hiding this comment.
This block (the RECEIVE_SPONSOR_CUSTOMIZED_FORM_ITEM path) has no tests, unlike the form reducer which got full coverage in this PR.
The file_path normalization here is the path that populates currentItem, which is what SponsorFormItemForm receives as initialValues — it's the edit-form image fix. Consider adding a test that asserts currentItem.images[].file_path === file_url, covering at least the images-present and images-absent cases.
| ...img, | ||
| file_path: img.file_url | ||
| })), | ||
| meta_fields: item.meta_fields.length > 0 ? item.meta_fields : [] |
There was a problem hiding this comment.
This PR defensively added (item.images || []) on line 113, but item.meta_fields on this line is still unguarded. If the API returns an item where meta_fields is null or undefined, this throws a TypeError and breaks the form-open flow.
Suggested fix:
meta_fields: (item.meta_fields ?? []).length > 0 ? item.meta_fields : []| @@ -43,10 +44,17 @@ const sponsorCustomizedFormReducer = (state = DEFAULT_STATE, action) => { | |||
| return DEFAULT_STATE; | |||
| } | |||
| case RECEIVE_SPONSOR_CUSTOMIZED_FORM: { | |||
There was a problem hiding this comment.
Two different action creators dispatch RECEIVE_SPONSOR_CUSTOMIZED_FORM into this case with different API expand contracts:
getSponsorManagedFormexpandsitems.images, so the normalization below runs correctly.getSponsorCustomizedFormdoes not expanditems, sopayload.response.itemsis absent and falls through the|| []guard safely.
This is safe today, but worth documenting — a future developer adding item-level display to the customized-form popup may not notice that items are empty in that path. Consider a comment here or splitting the action types to make the contract explicit.
| })), | ||
| meta_fields: item.meta_fields.length > 0 ? item.meta_fields : [] | ||
| }; | ||
| return { ...state, currentItem }; |
There was a problem hiding this comment.
No test file was added for this reducer, while the form reducer received full coverage in this PR. The three cases most worth adding:
RECEIVE_SPONSOR_CUSTOMIZED_FORM_ITEMwith images — assertscurrentItem.images[].file_path === file_url(the actual edit-form path this PR fixes)RECEIVE_SPONSOR_CUSTOMIZED_FORM_ITEMwithmeta_fields: undefined— proves the unguarded.lengthaccess on line 117 does not throwSPONSOR_FORM_MANAGED_ITEM_UPDATED— verifies that list images survive a save round-trip (currently storesupdatedItem.imageswithoutfile_pathnormalization, which is fine for table display but worth locking in)
…hared actions Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
* fix: adjust payload to customized managed sponsor forms Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add safety guards for items and arrays Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add items to default entity, unit test, map images on customized form item reducer Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add unit test cases, adjust reducer and add context comment on shared actions Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> --------- Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
ref: https://app.clickup.com/t/86b66n5jn
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests